Add path regex check to locks #174
Conversation
rules/engine.go
Outdated
| @@ -69,7 +70,7 @@ type V3Engine interface { | |||
| AddRule(rule DynamicRule, | |||
| lockPattern string, | |||
| callback V3RuleTaskCallback, | |||
| options ...RuleOption) | |||
| options ...RuleOption) error | |||
There was a problem hiding this comment.
Humm, this would be a major hit to all services to handle this new error.
Maybe we should consider a e.logger.Fatal(...) or a panic("Rule ID option missing") instead? That way only those that have issues will affected.
And then will need to make this a major version bump and an announcement in the armada-dev channel.
rules/engine.go
Outdated
| @@ -69,7 +70,7 @@ type V3Engine interface { | |||
| AddRule(rule DynamicRule, | |||
| lockPattern string, | |||
| callback V3RuleTaskCallback, | |||
| options ...RuleOption) | |||
| options ...RuleOption) error | |||
| AddPolling(namespacePattern string, | |||
There was a problem hiding this comment.
I think we might also have to check the AddPolling namespacePattern, but have not looked into that one yet.
There was a problem hiding this comment.
AddPolling calls AddRule so I think it should be okay unless the check should be higher up in the chain
Line 247 in 298d3d3
rules/engine.go
Outdated
| @@ -169,8 +170,13 @@ func (e *v3Engine) SetWatcherWrapper(watcherWrapper WrapWatcher) { | |||
| func (e *v3Engine) AddRule(rule DynamicRule, | |||
| lockPattern string, | |||
| callback V3RuleTaskCallback, | |||
| options ...RuleOption) { | |||
| options ...RuleOption) error { | |||
| validPath := regexp.MustCompile(`^[[:alnum:] \:\/\"\'\_\.\,\*\=\-]*$`) | |||
There was a problem hiding this comment.
This should be pushed out to a static var, no need to do this every time.
Maybe add a cmt with it to spell out the char to make it easier to know what is valid?
rules/engine.go
Outdated
| func (e *v3Engine) AddRule(rule DynamicRule, | ||
| lockPattern string, | ||
| callback V3RuleTaskCallback, | ||
| options ...RuleOption) { | ||
| e.addRuleWithIface(rule, lockPattern, callback, options...) | ||
| if !validPath.MatchString(lockPattern) { |
There was a problem hiding this comment.
Not including check that the path contains "lock" since /crawler/compliance-engine /armada-ingress/:region/clusters/:clusterid/ingress_update paths don't contain
There was a problem hiding this comment.
Humm, I think we should push to get that changed and then announce in armada-dev that this will be required from now on.
rules/engine.go
Outdated
| e.addRuleWithIface(rule, lockPattern, callback, options...) | ||
| if !validPath.MatchString(lockPattern) { | ||
| e.logger.Fatal("Path contains an invalid character") | ||
| } else { |
There was a problem hiding this comment.
No need for else, when above is fatal?
Issue https://github.ibm.com/alchemy-containers/armada-ballast/issues/2712 https://github.ibm.com/alchemy-containers/armada-ballast/issues/2802